Skip to content

chore(schematics): document the scrollbar-deprecated-path migration - #1907

Open
lskramarov wants to merge 3 commits into
mainfrom
chore/add-readme-for-migration
Open

chore(schematics): document the scrollbar-deprecated-path migration#1907
lskramarov wants to merge 3 commits into
mainfrom
chore/add-readme-for-migration

Conversation

@lskramarov

Copy link
Copy Markdown
Contributor

Summary

packages/schematics/src/migrations/scrollbar-deprecated-path/ was the only migration of 20 without a README.md, so it shipped to consumers undocumented while all its siblings ship a guide.

The missing file turned out to be more than a documentation gap. packages/schematics/scripts/copy-meta-to-dist.js copies each migration's README into the published package, its loop is sequential and rethrows, and the final rejection was swallowed — so the copy step aborted at the first missing file and the build still exited 0. Verified locally: with the README absent, dist/components/schematics/migrations/v20-upgrade/index.js and dist/components/schematics/utils/messages.js were never copied, yet yarn run build:schematics reported success. rimraf packages/schematics/dist does not clear dist/components, so a stale local build masks the gap while CI publishes the truncated package.

List of notable changes:

  • added README.md for the scrollbar-deprecated-path migration, following the structure of the existing ones — what the schematic rewrites, which specifiers the quote-anchored match deliberately skips, the fix default, and a before/after example.
  • documented the silent part of the entry-point split: @koobiq/components/scrollbar and @koobiq/components/scrollbar/deprecated share only KbqScrollbar and KbqScrollbarOptions, so most unmigrated imports fail with TS2305, while <kbq-scrollbar> keeps compiling and rendering against the new component and the [kbq-scrollbar] attribute form of the selector quietly matches nothing.
  • fixed copy-meta-to-dist.js so a copy failure fails the build: the catch now sets process.exitCode = 1, failures are collected instead of aborting the loop at the first one, and a missing README.md / schema.json is reported as No README.md for the <migration> migration instead of a raw ENOENT path.

Verified with yarn run build:schematics on a cleared dist/components (exit 0, all 20 migrations complete), a negative run with alert-signals/README.md and v20-upgrade/schema.json removed (exit 1, both reported by name, everything else still copied), and npx jest packages/schematics (21 suites, 275 tests).

What should reviewers focus on?

  • Whether the README's account of the behaviour change is what consumers should be told — in particular the claim that repointing the import preserves current rendering exactly, and that adopting the new scrollbar is a separate manual step.
  • Whether README.md and schema.json should be hard requirements for every migration directory. All 20 provide both today, so the new check is green, but it does mean a future migration cannot land without its README.

Copilot AI lite review requested due to automatic review settings August 17, 2026 14:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Visit the preview URL for this PR (updated for commit a8b1ec1):

https://koobiq-next--prs-1907-n7dlo2hv.web.app

(expires Thu, 20 Aug 2026 16:37:16 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/schematics/scripts/copy-meta-to-dist.js:97

  • The aggregated failure summary says files are "missing", but failures also includes non-missing copy failures from copyFileWrapper (e.g. permission/IO errors). This can mislead when debugging CI vs local issues; consider wording that covers both missing files and copy errors.
    if (failures.length > 0) {
        throw new Error([`${failures.length} file(s) missing from the package:`, ...failures].join('\n  '));
    }

@lskramarov
lskramarov requested a review from NikGurev August 17, 2026 16:44
@lskramarov lskramarov self-assigned this Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants